Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mock: change helper functions to expect::<thing> #2377

Merged
merged 2 commits into from
Nov 11, 2022
Merged

mock: change helper functions to expect::<thing> #2377

merged 2 commits into from
Nov 11, 2022

Conversation

hds
Copy link
Contributor

@hds hds commented Nov 11, 2022

Motivation

The current format of test expectations in tracing-mock isn't ideal. The format span::expect requires importing tracing_mock::<thing> which may conflict with imports from other tracing crates, especially tracing-core.

Solution

So we change the order and move the functions into a module called expect so that:

  • event::expect becomes expect::event
  • span::expect becomes expect::span
  • field::expect becomes expect::field

This format has two advantages.

  1. It reads as natural English, e.g "expect span"
  2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

use tracing_mock::field;

field::expect();

This import of the field module may then conflict with importing the same from tracing_core, making it necessary to rename one of the imports.

The same code would now be written:

use tracing_mock::expect;

expect::field();

Which is less likely to conflict.

This change also fixes an unused warning on MockHandle::new when the tracing-subscriber feature is not enabled.

Refs: #539

@hds hds requested review from hawkw, davidbarsky and a team as code owners November 11, 2022 16:49
@hds
mock: change helper functions to expect::<thing>
4de00e0 
The current format of test expectations in `tracing-mock` isn't ideal.
The format `span::expect` requires importing `tracing_mock::<thing>` which
may conflict with imports from other tracing crates, especially
`tracing-core`.

So we change the order and move the functions into a module called
`expect` so that:
* `event::expect` becomes `expect::event`
* `span::expect` becomes `expect::span`
* `field::expect` becomes `expect::field`

This format has two advantages.
1. It reads as natural English, e.g "expect span"
2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

```rust
use tracing_mock::field;

field::expect();
```

This import of the `field` module may then conflict with importing the
same from `tracing_core`, making it necessary to rename one of the
imports.

The same code would now be written:

```rust
use tracing_mock::expect;

expect::field();
```

Which is less likely to conflict.

This change also fixes an unused warning on `MockHandle::new` when the
`tracing-subscriber` feature is not enabled.

Refs: #539
@hawkw
Copy link
Member

hawkw commented Nov 11, 2022

looks like rustfmt needs to be run? https://github.com/tokio-rs/tracing/actions/runs/3446620438/jobs/5751783258

@hds
Copy link
Contributor Author

hds commented Nov 11, 2022

looks like rustfmt needs to be run? https://github.com/tokio-rs/tracing/actions/runs/3446620438/jobs/5751783258

Whoops. Thanks!

hawkw
hawkw approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new API looks great, thanks!

there's probably some places where imports in our existing tests can now be made a bit nicer, since we can import the tracing::{span, event} modules without clashing, but i think it's fine to not do that in this branch, especially since the diff is already quite large.

tracing-mock/src/expect.rs
Comment on lines +23 to +43
pub fn event() -> ExpectedEvent {
ExpectedEvent {
..Default::default()
}
}

pub fn field<K>(name: K) -> ExpectedField
where
String: From<K>,
{
ExpectedField {
name: name.into(),
value: ExpectedValue::Any,
}
}

pub fn span() -> ExpectedSpan {
ExpectedSpan {
..Default::default()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unrelated to this PR, but it occurs to me that these probably ought to have #[must_use] annotations on them, since calling these functions and dropping the result will do nothing. we should probably do a pass to add #[must_use] annotations as part of the release prep, in a follow-up PR...

@hawkw hawkw merged commit 22c1ed9 into master Nov 11, 2022
@hawkw hawkw deleted the hds/expect-thing branch November 11, 2022 17:53
@hawkw hawkw mentioned this pull request Nov 11, 2022
Merged
davidbarsky pushed a commit that referenced this pull request Sep 27, 2023
@hds @davidbarsky
mock: change helper functions to expect::<thing> (#2377)
dc636df 
* mock: change helper functions to `expect::<thing>`

The current format of test expectations in `tracing-mock` isn't ideal.
The format `span::expect` requires importing `tracing_mock::<thing>` which
may conflict with imports from other tracing crates, especially
`tracing-core`.

So we change the order and move the functions into a module called
`expect` so that:
* `event::expect` becomes `expect::event`
* `span::expect` becomes `expect::span`
* `field::expect` becomes `expect::field`

This format has two advantages.
1. It reads as natural English, e.g "expect span"
2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

```rust
use tracing_mock::field;

field::expect();
```

This import of the `field` module may then conflict with importing the
same from `tracing_core`, making it necessary to rename one of the
imports.

The same code would now be written:

```rust
use tracing_mock::expect;

expect::field();
```

Which is less likely to conflict.

This change also fixes an unused warning on `MockHandle::new` when the
`tracing-subscriber` feature is not enabled.

Refs: #539
@davidbarsky davidbarsky mentioned this pull request Sep 28, 2023
Merged
hawkw pushed a commit that referenced this pull request Oct 1, 2023
@hds @hawkw
mock: change helper functions to expect::<thing> (#2377)
5197803 
* mock: change helper functions to `expect::<thing>`

The current format of test expectations in `tracing-mock` isn't ideal.
The format `span::expect` requires importing `tracing_mock::<thing>` which
may conflict with imports from other tracing crates, especially
`tracing-core`.

So we change the order and move the functions into a module called
`expect` so that:
* `event::expect` becomes `expect::event`
* `span::expect` becomes `expect::span`
* `field::expect` becomes `expect::field`

This format has two advantages.
1. It reads as natural English, e.g "expect span"
2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

```rust
use tracing_mock::field;

field::expect();
```

This import of the `field` module may then conflict with importing the
same from `tracing_core`, making it necessary to rename one of the
imports.

The same code would now be written:

```rust
use tracing_mock::expect;

expect::field();
```

Which is less likely to conflict.

This change also fixes an unused warning on `MockHandle::new` when the
`tracing-subscriber` feature is not enabled.

Refs: #539
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
@hds @kaffarell
mock: change helper functions to expect::<thing> (tokio-rs#2377)
c6546d3 
* mock: change helper functions to `expect::<thing>`

The current format of test expectations in `tracing-mock` isn't ideal.
The format `span::expect` requires importing `tracing_mock::<thing>` which
may conflict with imports from other tracing crates, especially
`tracing-core`.

So we change the order and move the functions into a module called
`expect` so that:
* `event::expect` becomes `expect::event`
* `span::expect` becomes `expect::span`
* `field::expect` becomes `expect::field`

This format has two advantages.
1. It reads as natural English, e.g "expect span"
2. It is no longer common to import the modules directly.

Regarding point (2), the following format was previously common:

```rust
use tracing_mock::field;

field::expect();
```

This import of the `field` module may then conflict with importing the
same from `tracing_core`, making it necessary to rename one of the
imports.

The same code would now be written:

```rust
use tracing_mock::expect;

expect::field();
```

Which is less likely to conflict.

This change also fixes an unused warning on `MockHandle::new` when the
`tracing-subscriber` feature is not enabled.

Refs: tokio-rs#539
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hds @hawkw